Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement SSH Proxy Host #1688

Merged
merged 19 commits into from
Apr 28, 2024
Merged

Implement SSH Proxy Host #1688

merged 19 commits into from
Apr 28, 2024

Conversation

cgrinham
Copy link
Contributor

@cgrinham cgrinham commented Apr 14, 2024

Implemented options for using a proxy/bastion host with SSH backups.
First contribution so please let me know if you are happy with the style, I've tried to maintain the existing style but balance it with pep8 where possible. I think I caught all the places that are required for this but this is my first dive into this codebase so I wouldn't be surprised if I missed some things.

I separated out the ssh-copy-id command generation into its own function so I can run tests on it. I was thinking of doing the same for the ssh default args - let me know what you think.

image

This is my first time working with any desktop UI, more than open to suggestions of how it should work. I tried to be consistent with the rest of the dialog, however I think a checkbox or similar might be good for enabling these options.

Additionally, when doing a snapshot I found that because the backup method unmounts the mount at the end, the snapshot list no longer works and shows only "Now". If I comment the unmount out, it works as expected and shows the existing and new snapshots in the list. Is this a known issue?

@buhtz
Copy link
Member

buhtz commented Apr 15, 2024

Dear Christie,

Thank you very much for your contribution on behalf of the team and myself. I am pleased with the style, the unit tests, and your thoughtful refactoring. I appreciate your contribution.

Our team currently comprises three members, along with the former maintainer in a supporting role. However, apart from myself, all others are temporary unavailable for various reasons. Therefore, it may take some time before we can thoroughly discuss and decide on all the details of your PR.

First and foremost, I would like to gain a better understanding of the use case. I have not previously utilized an SSH proxy. Is it similar to a "jump host"? Additionally, could you share details of your real setup where this feature proves useful to you? Understanding your experience would greatly assist me in visualizing the use case myself.

The issue regarding "unmount" and the snapshot list requires investigation. Does this problem occur exclusively when using the proxy, or does it also manifest with "regular" SSH hosts?

We are dedicated to enhancing the codebase. Despite its current non-compliance with PEP8 standards, our goal remains to align with PEP8 guidelines, acknowledging its 15-year legacy and our position as the third generation of maintainers.

I will consider how best to enhance the dialogue, perhaps by visually disabling the SSH proxy section and enabling it through a checkbox. Ultimately, a redesign of the entire dialogue is necessary, but the current codebase is not conducive to such changes. Therefore, I intend to implement the solution in a simple manner and postpone the redesign.

Best regards,
Christian

EDIT: Note to myself: Have a look at #1298

@cgrinham
Copy link
Contributor Author

Hi Christian, thanks for your reply!
Yes this is exactly that - jump host. Wasn't sure which term to go with.
My set up is probably a bit unusual which is why my screenshot maybe doesn't explain it well.
I have a server at home (ProxyServer) and another at a relative's (BackupServer). BackupServer has a reverse tunnel to ProxyServer. I am backing up my laptop to BackupServer via ProxyServer. This way I don't have to open ports on the router at my relative's house.

A use case I have also seen would be to limit the attack vector of a backup server by eg. accepting only requests from a certain IP address (the proxy server).

A simpler example would be:
image

The issue regarding "unmount" and the snapshot list requires investigation. Does this problem occur exclusively when using the proxy, or does it also manifest with "regular" SSH hosts?
I tested with and without my changes and the issue persisted on both.

I would be very keen to help with some refactoring work - I can identify some small places to start to show what I'd do then would be great to get some feedback about whether people are happy with my contributions. Very keen to add more tests also.

@buhtz buhtz self-assigned this Apr 19, 2024
@buhtz
Copy link
Member

buhtz commented Apr 19, 2024

I am taking it from here, working on your branch and will come up with some modifications in the next days.

Sidenote about translation and localization:

QLabel(_('Port') + ':', self)

In most cases it makes more sense to make punctuations and other signs part of the string to translate. This make sense for right-to-left (RTL) languages and bidirectional-reading (BIDI) environments.

@cgrinham
Copy link
Contributor Author

@buhtz okay cool thanks
re: translation, I just copied it from further up the code but will keep that in mind and good to know

@buhtz
Copy link
Member

buhtz commented Apr 20, 2024

Hello Christie,
I have a side question. Do you somehow modified the file mode (file permissions) of the files in the repo? Can you please cd into your local repos folder and show me the output of git config --list --show-scope --show-origin | grep filemode. And the output of ls -l updatecopyright.sh please. Thanks.

@buhtz
Copy link
Member

buhtz commented Apr 21, 2024

This is how it looks like and behave now.
Peek 2024-04-21 08-49

The label "SSH Proxy (optional)" need to be modified. I would propose to use "SSH Proxy" (the term proxy is used in the man page, too) and add a tooltip to the whole widget explaining the purpose, use alternative term "Jump host" and reference the command switches used (e.g. ssh -J).

Needs discussion

I also vote to finally remove TestSshKey.test_sshCopyId() (#1298). A system test, runs only on Germars machine. There is also not much value in it but has lot of maintenance costs. It simply tests an extern tool which has not much value.

Open problems

The thing with the lost snapshot list needs further investigation.

@buhtz buhtz added Discussion decision or consensus needed Bug labels Apr 21, 2024
@cgrinham
Copy link
Contributor Author

Hello Christie, I have a side question. Do you somehow modified the file mode (file permissions) of the files in the repo? Can you please cd into your local repos folder and show me the output of git config --list --show-scope --show-origin | grep filemode. And the output of ls -l updatecopyright.sh please. Thanks.

Not knowingly!
here is the output:

local	file:.git/config	core.filemode=true
-rwxrwxr-x 1 christie christie 344 Apr  1 20:31 updatecopyright.sh

@cgrinham
Copy link
Contributor Author

The label "SSH Proxy (optional)" need to be modified. I would propose to use "SSH Proxy" (the term proxy is used in the man page, too) and add a tooltip to the whole widget explaining the purpose, use alternative term "Jump host" and reference the command switches used (e.g. ssh -J).

New UI looks great - I agree with the checkbox the "(optional)" is no longer needed.

Needs discussion

I also vote to finally remove TestSshKey.test_sshCopyId() (#1298). A system test, runs only on Germars machine. There is also not much value in it but has lot of maintenance costs. It simply tests an extern tool which has not much value.

I would agree - these tests are also quite hard to read as a newcomer to the codebase

@buhtz
Copy link
Member

buhtz commented Apr 22, 2024

Here is the new visual with cleaned label and a tool tip. Any re-wording suggestions?
image

@cgrinham
Copy link
Contributor Author

Here is the new visual with cleaned label and a tool tip. Any re-wording suggestions? image

Looks good to me

@buhtz
Copy link
Member

buhtz commented Apr 22, 2024

Additionally, when doing a snapshot I found that because the backup method unmounts the mount at the end, the snapshot list no longer works and shows only "Now". If I comment the unmount out, it works as expected and shows the existing and new snapshots in the list. Is this a known issue?

I need to setup a jump host scenario in a VM to investigate this further.

  • Does this problem occur exclusively when using the proxy, or does it also manifest with "regular" SSH hosts?
  • Can you reproduce the problem? Please run BIT with --debug argument and show me the terminal output.
  • If the time line do not dissapear without using an SSH proxy then I hypothesize that qt/app.py::FillTimeLineThread() and common/snapshots.py::iterSnapshots() are somehow involved. They do not fail if nothing is mounted. They are simply silent. When a regular (without jump host) SSH snapshots was taken there might be a re-mount somewhere hidden in the code we do not know know about? Maybe the debug output will give us a hint about it.

EDIT: The failing TravisCI is an issue on their own servers and not related to this PR.

@cgrinham
Copy link
Contributor Author

cgrinham commented Apr 22, 2024

  • Does this problem occur exclusively when using the proxy, or does it also manifest with "regular" SSH hosts?

This happened with any SSH host.

Will try get you the output later today - I tested on the main branch and I saw the same behaviour

@buhtz
Copy link
Member

buhtz commented Apr 22, 2024

If you are able to reproduce the problem with the snapshot timeline on "main" branch independent from this PR then I would suggest you to report it in a separate issue.

@buhtz buhtz marked this pull request as ready for review April 23, 2024 06:56
@buhtz buhtz added PR: Waiting for review PR won't be merged until review and approval from a member of the maintenance team. and removed Discussion decision or consensus needed labels Apr 23, 2024
@buhtz
Copy link
Member

buhtz commented Apr 28, 2024

I tested on the main branch and I saw the same behaviour

Do you tested it on the branch named main or on the primary/default branch of the repository named dev? Please report back at #1705 where I track this specific issue.

I will merge the PR because the bug you describe is not reproducible. I setup a jumps host and it worked fine. Because the bug is not critical I decide to merge this PR but keep the separate issue #1705 open for a while.

@buhtz buhtz merged commit ba1ff78 into bit-team:dev Apr 28, 2024
1 check passed
@cgrinham
Copy link
Contributor Author

I tested on the main branch and I saw the same behaviour

Do you tested it on the branch named main or on the primary/default branch of the repository named dev? Please report back at #1705 where I track this specific issue.

I will merge the PR because the bug you describe is not reproducible. I setup a jumps host and it worked fine. Because the bug is not critical I decide to merge this PR but keep the separate issue #1705 open for a while.

I tested it on the dev branch - haven't tried the main branch.

Okay I'll try get some time to reproduce it this week.

Thanks for merging!

buhtz added a commit that referenced this pull request Jun 2, 2024
Fix crash with SSHProxyWidget when port was of type int.

Bug was introduced with #1688
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug PR: Waiting for review PR won't be merged until review and approval from a member of the maintenance team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants